Skip to content

fix(sequentialthinking): bound the stderr thought box - #5435

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/fix/5433-sequentialthinking-linear-thought-box
Nov 4, 2026
Merged

cliffhall merged 3 commits into
v2/mainfrom
v2/fix/5433-sequentialthinking-linear-thought-box

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #5433

Description

The stderr thought box from #5205 padded every row to the thought's longest line, so a thought with one long line and many short ones logged their product. Copilot found it reviewing the v2.3.0 milestone merge PR #5430: a 60 KB thought wrote 400 million characters and pushed RSS to 878 MB. A somewhat larger one would exhaust memory, from a single valid call with logging on by default.

The box is now bounded:

  • Width: at most MAX_BOX_TEXT_WIDTH (100). Longer lines wrap onto further rows, and so does a header longer than that (a long branchId, which would otherwise set every row's width). A cut never splits a surrogate pair.
  • Height: at most MAX_BOX_ROWS (1000) rows of thought text. Past that the box ends with one row, … N more lines not shown. Padding every short line to the width would otherwise still log about 100 times the thought's size.

#5205's behaviour is kept: one row per line, any line ending ends a row, no \r inside a row, every row the same width (#5108), and colour sized from the visible header (#5000). Only stderr changes. The stored thought and the tool result are unchanged.

Server Details

  • Server: sequentialthinking
  • Changes to: the thought log on stderr (lib.ts, formatThought); README; patch changeset

Motivation and Context

#5433. It is a regression from #5205, in this release (v2.3.0, #5425), so it is fixed before the release.

How Has This Been Tested?

Measured on the module (SequentialThinkingServer.processThought, stderr counted). The thought is one line of n characters, then n - 1 one-character lines:

n thought candidate cc602786 this branch
5,000 15 KB 25,055,028 chars, 112 MB RSS —
20,000 60 KB 400,220,028 chars, 878 MB RSS 107,535 chars, 63 MB RSS
200,000 600 KB — 107,535 chars, 70 MB RSS
2,000,000 6 MB — 107,535 chars, 123 MB RSS

Inspector CLI 2.13.0, tools/call sequentialthinking with the n = 5,000 thought, both eras:

candidate legacy  exit=0 {"thoughtNumber":1,"thoughtHistoryLength":1} stderr 25,105,119 chars, 5,006 lines
candidate modern  exit=0 {"thoughtNumber":1,"thoughtHistoryLength":1} stderr 25,105,119 chars, 5,006 lines
branch    legacy  exit=0 {"thoughtNumber":1,"thoughtHistoryLength":1} stderr 112,232 chars, 1,007 lines, "… 4049 more lines not shown"
branch    modern  exit=0 {"thoughtNumber":1,"thoughtHistoryLength":1} stderr 112,232 chars, 1,007 lines, "… 4049 more lines not shown"

LLM client: Claude Code 2.1.318, headless, --strict-mcp-config naming this branch's build (no era switch). It was asked for one sequentialthinking call with a 300-character line and three short lines, and returned {"thoughtNumber":1,"totalThoughts":1,"nextThoughtNeeded":false,"branches":[],"thoughtHistoryLength":1}.

Tests: thought-logging.test.ts adds:

  • wrapping at the capped width;
  • Copilot's exact case staying bounded;
  • the row cap's summary row, including a line the cap cuts partway;
  • a thought that exactly fills the cap, with no summary row;
  • every row the same width;
  • a long branchId header wrapping and staying linear;
  • an emoji at the cut not split.

The #5205 "very many lines" test now expects the capped box. The suite is 138 passed; lib.ts coverage is 100 / 98.11 / 100 / 100. npm run local:gate: EXIT=0.

Breaking Changes

None. Only the stderr diagnostic's layout changes for thoughts with lines over 100 characters or more than 1000 rows.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation (not applicable to the protocol surface: stderr only)
  • My changes follow MCP security best practices
  • I have updated the server's README accordingly
  • I have added a changeset (npm run changeset) if this changes what a TypeScript server publishes
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling (not applicable: no new failure path)
  • I have documented all environment variables and configuration options (not applicable: none added)

🤖 Generated with Claude Code

#5205 padded every row of the box to the thought's longest line, so one
long line and many short ones logged their product: a 60 KB thought wrote
400 million characters. The box is now at most MAX_BOX_TEXT_WIDTH (100)
wide, wrapping longer lines and a longer header without splitting a
surrogate pair, and at most MAX_BOX_ROWS (1000) rows, ending with a row
that counts the lines left out. The box tops out near 107 KB; the stored
thought and the tool result are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall cliffhall added the v2 label Nov 4, 2026
@changeset-bot

changeset-bot Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c583da0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modelcontextprotocol/server-sequential-thinking Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Truncated thoughts containing only short lines can produce a summary row wider than the box, so box sizing must account for the summary and tests must verify consistent row widths.

1 open issue
What changed in this PR

Fixes the sequentialthinking stderr logging regression from #5205 by limiting thought-box expansion, addressing #5433 without changing stored thoughts or tool results.

Changes:

  • Caps text width at 100 UTF-16 code units and thought text at 1,000 rows, with wrapping and an omission summary.
  • Adds regression tests for large inputs, header wrapping, row alignment, and surrogate-pair preservation.
  • Documents the logging limits and adds a patch changeset.
File Description
src/​sequentialthinking/​README.md Documents thought-log wrapping and truncation.
src/​sequentialthinking/​lib.ts Implements width limits, wrapping, and thought-row truncation.
src/​sequentialthinking/​__tests__/​thought-logging.test.ts Tests bounded output and wrapping edge cases.
.changeset/​sequentialthinking-5433-bounded-thought-box.md Records the logging fix for a patch release.

Review effort: Balanced

Comment thread src/sequentialthinking/lib.ts Resolved
A truncated thought of short lines had a summary row wider than the box
(33 characters against a 20-character border). The row texts are now
collected first and the box is sized to include the summary; widening
cannot change the wrapping, since below the cap no line wrapped and at
the cap the summary fits. The very-many-lines test checks every row's
width.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1: one finding, fixed in 43d93af: a truncated thought of short lines had a summary row wider than the box. Re-requesting review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The documentation incorrectly promises returned thought text and absolute output limits, although results contain only metadata and unbounded branch IDs can exceed the stated limits.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Low severity Qualify output limits to account for unbounded branch IDs

.changeset/​sequentialthinking-5433-bounded-thought-box.md:5

The 1000-row cap applies only to thought text; every wrapped header row is still rendered. Since branchId has no length limit, a sufficiently long ID exceeds the claimed total height and 107 KB maximum. Qualify the limit as applying to thought-text rows and remove the absolute output-size guarantee; the same height claim in src/sequentialthinking/README.md:137 also needs qualification.

Low severity Clarify thought text is retained in history, not returned by the tool

src/​sequentialthinking/​README.md:137

The thought text is not returned by this tool: processThought returns only counters, flags, and branch IDs, and index.ts exposes the same metadata as structured content. Saying it is “returned whole” incorrectly promises that clients can retrieve text omitted from stderr. Describe the complete thought as retained in history and the tool result as unchanged instead.

1 resolved since last review

Review effort: Balanced

The row cap counts thought-text rows only (a long branchId's header rows
are not capped), so there is no absolute size guarantee; and the tool
result never carries the thought text, which the server keeps whole in
its history. README, changeset and the MAX_BOX_ROWS comment now say so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 2: no inline findings; two "previously missed" findings in the review body (no thread to reply in), both doc accuracy, both fixed in c583da0:

  • Output limit overstated. The 1000-row cap counts thought-text rows only; header rows from a long branchId are wrapped but not capped, so "about 107 KB at most" was not absolute. The README, changeset and MAX_BOX_ROWS comment now say "at most 1000 rows of thought text" and drop the absolute size claim.
  • "Returned whole" was wrong. The tool result carries no thought text (counters, flags, branch IDs). The docs now say the server keeps the whole thought in its history and the tool result is unchanged.

local:gate EXIT=0. Re-requesting review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block approval of the change to bound the sequentialthinking server’s stderr thought-box output.

No open issues 🎉

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 3: clean (no findings, no suppressed comments). Review loop ends on the first clean round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sequentialthinking: the multi-line thought box grows quadratically and can exhaust memory

2 participants